init: import whichtests sources#1
Conversation
Imports the testselect Go test-plan generator from coder/coder@bf6a0d953f (branch ethan/go-test-flake-detector) and splits the single ~1.6k-line main.go into ten focused files inside package main: - cli.go: entrypoint, flag parsing, command orchestration - config.go: config / commandConfig types and defaults - request.go: runRequest, diffRange, revision validation - gitexec.go: gitRunner / gitFetcher types and exec.Command impl - diff.go: git diff parsing, change kinds, hunks, line ranges - snapshot.go: AST snapshot parsing, fileSnapshot, sharedDecl, fallbacks - broadening.go: per-kind broadening rules (broadeningScope) - selection.go: per-change selection logic - inventory.go: inventoryCache for package/directory test discovery - plan.go: plan construction, matrix and summary rendering githubactions.go and publish.go are imported unchanged. Tests (githubactions_test.go, integration_test.go, main_test.go) are kept as single files initially. go vet, go build, go test, and go test -race are all green.
Split main_test.go (2297 lines) into focused *_test.go files mirroring the source layout, and move two helpers (mergePackageSelection, packagePattern) from plan.go to selection.go so layering is consistent (plan depends on selection, not vice versa). Test files now line up 1:1 with their production counterparts: cli_test.go - run/runCommand end-to-end tests diff_test.go - hunk/change parsing tests snapshot_test.go - parseFileSnapshot/fallback tests selection_test.go - selectTestsForSnapshots and merge tests plan_test.go - buildExecutionPlan/renderSummary tests publish_test.go - publishPlan tests (from githubactions_test.go) githubactions_test.go - GitHub event/range tests integration_test.go - real-git integration (unchanged) helpers_test.go - shared line-range + inventory helpers gitfake_test.go - fakeGitRepo mock + dispatcher methods go vet, go build, go test, and go test -race all pass.
Mirrors the parallel testctx repo's structure: Makefile, golangci-lint v2 config, prettier config, gitignore, MIT license, CI workflow with fmt/lint/test jobs, and the composite setup-go action (default Go version bumped to 1.26.2 to match go.mod). Drive-by gofumpt fix: blank line between needsOldSnapshot and addMatchingTests in selection.go.
- publish.go: surface Close errors via named return on appendFile, fixing errcheck. Close errors on a write path can indicate lost data, so propagating beats silently discarding. - plan.go: drop `:=` in selectTestPlan loop so the inner err reuses the outer binding, fixing govet shadow. - githubactions.go: drop the redundant cfg.config selector and rely on the promoted method, fixing staticcheck QF1008. - helpers_test.go: drop both dir and packageName parameters from mustPackageInventory; all 16 call sites pass the same synthetic values, so hardcode them and document the helper, fixing unparam. go vet, go build, make test, and make fmt are clean. make lint still reports six gosec findings (G301/G304) covering directory permissions and reads from user-provided paths; those are policy decisions.
The previous name was honest but generic — many things 'select tests.' 'whichtests' is the question this tool answers: given a Go PR's diff, which tests should run? Renaming makes the intent obvious in workflow YAML, CLI help, and the module path. Touchpoints: - go.mod module github.com/coder/whichtests - Makefile builds build/whichtests - cli.go doc comment - README.md heading and invocation examples Internal symbol names like selectTestsForSnapshots are untouched — they describe what the function does, not the tool's identity.
ethanndickson
left a comment
ethanndickson
left a comment
There was a problem hiding this comment.
Thanks for extracting this from coder/coder. The algorithm is small, the git/AST/matrix boundaries are kept clean, and the regex gates on safeTestNameRE/safePackagePatternRE plus the GITHUB_OUTPUT CRLF + 1 MiB guard close the highest-impact injection paths. The two real-git integration tests are a particularly strong asset.
Reviewer context: this binary has exactly one consumer — coder/coder's flake-go.yaml, invoked as whichtests --repo-root . --github-actions --out-matrix "$RUNNER_TEMP/flake-matrix.json" after a per-PR go install from a pinned SHA (no binary caching). Severity is calibrated to that flow.
11 reviewers in parallel cross-checked into 6 P2, 8 P3, 1 perf bundle, 2 nit bundles across 17 inline comments. Most P2s are deletions (dead config, fossil fields, a test-only run() parallel entry that 9 reviewers independently flagged) plus two algorithm findings the smoke test exposed. None block merge.
ethanndickson
left a comment
ethanndickson
left a comment
There was a problem hiding this comment.
Round 1 cleanups all landed cleanly at 4acef74. The testDecl flattening, run() shim removal, fallback-parsing deletion, pull_request.head.sha strictness, broadening max-scope correction, summary filename quoting, and xerrors removal are all in place; the resulting code is noticeably smaller and clearer.
Round 2 findings are simplification opportunities in the same vein: dead struct fields (matching the round-1 testDecl pattern), single-use helpers, double-AST-parsing with unreachable fallback branches, and a defaults function pair that risks drift. 9 P3, 4 nits, 1 obs across 14 inline comments. None block merge.
ethanndickson
left a comment
ethanndickson
left a comment
There was a problem hiding this comment.
Follow-up to the round-2 review: one P3 simplification that I previously catalogued as an observation. On reflection, since PR #1 is the initial import, every line lands now, so the "future-shape" framing was the wrong lens. This one is a real simplification with clear safety, so promoting it to P3 and posting inline.
Apply deletion-first cleanups identified in the round 2 deep review: - gitexec: shrink gitResult to just Stdout; keep stderr/exit local to execGit - config: collapse defaultConfig through withDefaults via cmp.Or - publish: inline the trailing-newline append at its single call site - snapshot: stop caching testingDotImport on fileSnapshot; pass it directly - plan: drop unused executionAccumulator.Package, fold appendUniqueNote, rename unsafeRunRegexTestNames -> unsafeRunRegexTestCount - diff: inline oldRevisionPath/newRevisionPath into pathspecs; fold isRunnableGoTestPath into isRunnableTestFilePath - selection: parse each changed file once via parsedFileSnapshot; thread cache through selectChange instead of re-passing cfg/git; drop unreachable parse fallback branches; simplify allDirectoryTestsSelection
ethanndickson
left a comment
ethanndickson
left a comment
There was a problem hiding this comment.
Round 3 cleanup landed cleanly at f3da5e5 — every Resolved finding from rounds 1 and 2 matches the new code, and the deferred testFileChange.Kind observation still looks like the right call to defer.
Fresh review across 11 reviewer roles: 8 P3, 4 Obs, and 1 grouped Nit across 13 inline comments. The two convergent findings are readFileAtRevision per-file caching (5 reviewers; round 1 cited this cost but the caching that landed addressed package inventories, not the per-file git work) and appendGitHubOutput's outputSizeLimit test-only seam (3 reviewers; same shape round 1 removed from runRequest.OutputSizeLimit).
1 participant
Initial import of the
whichteststool, extracted fromcoder/coder'sscripts/testselect/and renamed.whichtestsanswers: given a PR diff, which Go tests should run? The flake-go workflow uses its output to rerun new/modified tests under shuffle and surface flakes early.Commits
init: importscripts/testselectfromcoder/coderand splitmain.goacross cohesive files (cli, config, request, gitexec, diff, snapshot, broadening, selection, inventory, plan, publish, githubactions)split: organize tests along source-file boundaries (1:1 with sources)chore: import boilerplate fromcoder/paralleltestctx(.gitignore, .golangci.yaml, .prettierrc, LICENSE, Makefile, CI workflow, setup-go composite action)fix: address mechanical lint findings (errcheck, govet shadow, staticcheck QF1008, unparam)rename: testselect -> whichtests (module path, Makefile target, CLI doc comment, README)Outstanding
gosecfindings (G304/G301) deferred for review. Recommendation is to lowerpublish.goMkdirAll perms to0o750and#nosecannotate the remaining 4 sites (test temp dir, GITHUB_EVENT_PATH, --out-matrix and --out-summary flag values).coder/coder'sscripts/testselect/directory and theflake-go.yamlreference still need to be migrated to use this repo; that's out of scope for this PR.Relates to CODAGT-381